fix: inherit HideHelpCommand in subcommands - #2395
Conversation
dearchap
left a comment
dearchap
left a comment
There was a problem hiding this comment.
Reviewed the change. Looks correct — the new test fails without the fix and passes with it, and test suite/vet/gofmt are clean. Two notes below (one behavior change to confirm, one minor doc touch-up).
| if !cmd.hideHelp() { | ||
| if cmd.Command(helpCommand.Name) == nil { | ||
| if !cmd.HideHelpCommand { | ||
| if !cmd.hideHelpCommand() { |
There was a problem hiding this comment.
Heads-up: this is a behavior change for nested trees, not just a bug fix. hideHelpCommand() walks the whole ancestor chain, so once any ancestor sets HideHelpCommand: true, a descendant cannot re-enable the built-in help command — even by explicitly setting HideHelpCommand: false on itself. Before this change, app config help still worked when only the root hid the command, because each command's own field value controlled the append. If that was intentional (matching HideHelp inheritance semantics), consider noting it explicitly in the release notes.
| return false | ||
| } | ||
|
|
||
| func (cmd *Command) hideHelpCommand() bool { |
There was a problem hiding this comment.
Minor: with this change HideHelpCommand now propagates to all descendants, but its doc comment in command.go still reads // Ignored if HideHelp is true. only. A short note that the value applies to this command and its subcommands would prevent confusion about why a nested command lost its help command.
The field now behaves like HideHelp: a true value applies to the whole subtree and a subcommand cannot turn it back off. Say so on the field and pin both halves of that behaviour with tests, so the shared semantics are not lost again.
|
Yes, that's intentional. HideHelp already works this way, it walks up the parent chain, so a subcommand setting HideHelp: false under a hidden root doesn't get help back either. This just makes HideHelpCommand behave the same as HideHelp. I added two tests for it so it doesn't get flipped back by accident later. Doc comment updated as you asked. I also had to update godoc-current.txt and testdata/godoc-v3.x.txt, since CI compares those against the source. For the release note, maybe something like: HideHelpCommand is now inherited by subcommands, same as HideHelp. If a command sets it, the help command is hidden for that command and everything under it. |
dearchap
left a comment
dearchap
left a comment
There was a problem hiding this comment.
LGTM. The fix is correct: hideHelpCommand() mirrors hideHelp() by walking the parent chain, and the parent pointers are guaranteed to be set before ensureHelp() runs for any subcommand (setupDefaults sets direct children parents before ensureHelp; setupCommandGraph uses a pre-order Walk). Verified locally: full suite + race pass, vet/gofmt clean, generated godoc matches the updated godoc-current.txt and testdata/godoc-v3.x.txt.
2 participants
What type of PR is this?
What this PR does / why we need it:
HideHelpCommandinherit through the command hierarchy, matching the existing behavior ofHideHelp.helpcommand when an ancestor disabled it.--helpremains available.Which issue(s) this PR fixes:
Fixes #2129
Testing
go test ./... -count=1go run scripts/build.go generatego run scripts/build.go vetgo run scripts/build.go check-binary-sizego run scripts/build.go gfmrunRelease Notes